-
Notifications
You must be signed in to change notification settings - Fork 205
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Module/form manager #2098
base: develop
Are you sure you want to change the base?
Module/form manager #2098
Conversation
… block instead of generic sub section
…nd made Single Step Product Edit page as default refactor: replaced product add/edit page code with OOP fix: added Dokan Pro checks for `product_vendors_can_create_tags` and `product_category_style` admin settings
… dokan_product_edit_meta_data, this will save some executions refactor: removed update_post_meta and get_post_meta references
…Manager.php to prevent direct access to the files
…o module/form_manager
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (3)
assets/src/less/products.less (1)
472-499
: LGTM: Well-structured form layout implementation.The shipping dimension options layout is well-organized with proper spacing and width calculations. The 23.3% width with 2% margin for form groups ensures proper alignment in the grid system.
Consider using CSS custom properties (variables) for the repeated values:
.dokan-shipping-dimention-options { .dokan-form-group { - width: 23.3%; + width: var(--form-group-width, 23.3%); - margin-right: 2%; + margin-right: var(--form-group-margin, 2%); } }includes/Assets.php (1)
1040-1063
: Track refactoring of variation-related strings.The variation-related strings are properly implemented, but there's a TODO comment indicating they should be moved to Dokan Pro Assets class.
Would you like me to create a GitHub issue to track the planned refactoring of these strings to the Dokan Pro Assets class?
includes/ProductForm/Init.php (1)
269-269
: Address the TODO: Add support for creating tagsThere's a TODO comment indicating that support for creating tags needs to be added. Implement this functionality to enhance the product tagging feature.
Would you like assistance in implementing support for creating tags, or should I open a GitHub issue to track this task?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
assets/src/less/products.less
(3 hunks)includes/Assets.php
(8 hunks)includes/ProductForm/Init.php
(1 hunks)
🔇 Additional comments (8)
assets/src/less/products.less (1)
284-286
: LGTM: Clean utility class implementation.
The .dokan-hide
class follows standard naming conventions and uses the correct CSS property for hiding elements.
includes/Assets.php (4)
9-9
: LGTM: Import statement follows PSR-4 standard.
The import statement for ProductFormFactory
is properly namespaced and follows PSR-4 autoloading conventions.
522-522
: LGTM: Proper dependency management.
The addition of 'jquery-tiptip' as a dependency is appropriate as it's a WooCommerce-provided script needed for tooltip functionality.
979-993
: LGTM: Proper internationalization of UI strings.
The added localization strings are:
- Properly internationalized using WordPress i18n functions
- Correctly escaped where necessary
- Clear and user-friendly
1209-1209
: LGTM: Proper filter implementation with sensible default.
The product types filter is properly implemented with 'simple' as the default value, allowing for easy extension through the WordPress filter system.
includes/ProductForm/Init.php (3)
581-583
: Check sanitize_callback
parameters for Downloads field
The sanitize_callback
for the Downloads field accepts multiple parameters ($file_names
, $file_urls
, $file_hashes
), which may not align with the expected callback signature. Ensure that the callback function receives the appropriate parameters as per the implementation.
Please confirm that the form handling logic passes these parameters correctly to the sanitize_callback
. If not, adjust the function to accept a single $value
parameter or modify the form processing accordingly.
92-94
: Verify sanitize_callback
parameters for Slug field
The sanitize_callback
for the Slug field accepts multiple parameters ($slug
, $product_id
, $status
, $post_parent
), which may not match the expected callback signature. Ensure that this function is called with the correct parameters during form processing.
Confirm that the form processing code provides these parameters to the sanitize_callback
. If not, adjust the callback to match the expected usage.
223-231
: Ensure proper parameter passing in sanitize_callback
for Categories field
The sanitize_callback
function for the Categories field accepts $categories
and $product
as parameters. Verify that the form processing logic supplies these parameters when invoking the callback.
Please ensure that the callback receives the correct parameters during execution. If necessary, adjust the form handling code to pass the required arguments.
@@ -972,7 +998,7 @@ | |||
|
|||
.instruction-inside { | |||
text-align: center; | |||
padding-top: 40px; | |||
padding-top: 80px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codebase verification
Padding change appears inconsistent with responsive design patterns
The codebase shows multiple declarations of .instruction-inside
with different padding approaches:
- One instance uses percentage-based padding (20% top, 30% bottom)
- Another uses the new fixed 80px padding-top
- These different padding styles appear in the same file without clear responsive breakpoints
The change from percentage-based padding to fixed pixels could cause layout issues across different viewport sizes, especially since the file already contains mobile-specific media queries (@media max-width: 430px).
Recommend:
- Maintain consistent padding strategy (either percentage or fixed)
- If fixed padding is required, add appropriate media queries for smaller screens
- Consider reverting to percentage-based padding for better responsiveness
🔗 Analysis chain
Verify the increased padding impact on layout.
The padding-top has been significantly increased from 20px to 80px. While this might improve visual spacing, please verify that it doesn't cause any layout issues or excessive whitespace in different viewport sizes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any responsive design considerations for the instruction-inside class
# Look for media queries or responsive adjustments that might affect this padding
rg -A 5 "instruction-inside.*{" --type less
rg "@media.*{" --type less
Length of output: 262
Script:
#!/bin/bash
# Fix the regex syntax and search for instruction-inside class and media queries
rg -A 5 "instruction-inside" --type less
rg "@media" --type less
Length of output: 4309
* @since DOKAN_SINCE | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace placeholder 'DOKAN_SINCE' with the actual version number
The @since DOKAN_SINCE
tag in the class docblock should be replaced with the actual version number before release for accurate documentation.
* @since DOKAN_SINCE | ||
* | ||
* @return void |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update @since DOKAN_SINCE
in method docblock
The @since DOKAN_SINCE
tag in the __construct()
method docblock should be updated with the appropriate version number.
* @since DOKAN_SINCE | ||
* | ||
* @return void | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure @since DOKAN_SINCE
tags are updated
All occurrences of @since DOKAN_SINCE
in the method docblocks should be replaced with the actual version number to maintain accurate documentation.
Also applies to: 45-48, 55-58, 88-91, 107-110
includes/ProductForm/Init.php
Outdated
Elements::DOWNLOADABLE, [ | ||
'title' => __( 'Downloadable', 'dokan-lite' ), | ||
'description' => __( 'Virtual products are intangible and are not shipped.', 'dokan-lite' ), | ||
'field_type' => 'checkbox', | ||
'name' => '_downloadable', | ||
'additional_properties' => [ | ||
'value' => 'yes', | ||
], | ||
'sanitize_callback' => function ( $value ) { | ||
return ! empty( $value ) && 'yes' === $value; | ||
}, | ||
] | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent use of checkbox values: 'yes' vs 'on'
The checkboxes for Elements::DOWNLOADABLE
and Elements::VIRTUAL
use 'value' => 'yes'
in additional_properties
, and the sanitize_callback
checks for 'yes'
. However, the dependency_condition
checks for 'value' => 'on'
. This inconsistency may lead to dependency conditions not working as expected.
Update the dependency_condition
values to match the checkbox values:
// For Date On Sale From field
- 'value' => 'on',
+ 'value' => 'yes',
// For Date On Sale To field
- 'value' => 'on',
+ 'value' => 'yes',
// For Downloadable Options section
- 'value' => 'on',
+ 'value' => 'yes',
Also applies to: 368-381
includes/ProductForm/Init.php
Outdated
'title' => __( 'Downloadable', 'dokan-lite' ), | ||
'description' => __( 'Virtual products are intangible and are not shipped.', 'dokan-lite' ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Descriptions for 'Downloadable' and 'Virtual' fields are swapped
The description
for the Downloadable
field describes virtual products, and the description
for the Virtual
field describes downloadable products. Please swap the descriptions to match the correct fields.
Apply this diff to fix the descriptions:
// For the 'Downloadable' field
- 'description' => __( 'Virtual products are intangible and are not shipped.', 'dokan-lite' ),
+ 'description' => __( 'Downloadable products give access to a file upon purchase.', 'dokan-lite' ),
// For the 'Virtual' field
- 'description' => __( 'Downloadable products give access to a file upon purchase.', 'dokan-lite' ),
+ 'description' => __( 'Virtual products are intangible and are not shipped.', 'dokan-lite' ),
Also applies to: 370-371
/** | ||
* Maximum number of tags a vendor can add. | ||
* | ||
* @since 3.3.7 | ||
* | ||
* @args int -1 default to unlimited | ||
*/ | ||
$maximum_tags_select_length = apply_filters( 'dokan_product_tags_select_max_length', -1 ); | ||
|
||
if ( $maximum_tags_select_length !== -1 && count( $tags ) > $maximum_tags_select_length ) { | ||
// translators: %s: maximum tag length | ||
return sprintf( __( 'You can only select %s tags', 'dokan-lite' ), number_format_i18n( $maximum_tags_select_length ) ); | ||
} | ||
|
||
// todo: add support for creating tags | ||
return array_map( | ||
function ( $tag ) { | ||
if ( is_numeric( $tag ) ) { | ||
return absint( $tag ); | ||
} else { | ||
return sanitize_text_field( $tag ); | ||
} | ||
}, (array) $tags |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Handle errors properly in sanitize_callback
for Tags field
In the sanitize_callback
function for the Tags field, returning a string error message is not appropriate for error handling. Instead, consider returning a WP_Error
object or using a validation mechanism to handle the error.
Apply this diff to correct the error handling:
- return sprintf( __( 'You can only select %s tags', 'dokan-lite' ), number_format_i18n( $maximum_tags_select_length ) );
+ return new \WP_Error(
+ 'too_many_tags',
+ sprintf( __( 'You can only select %s tags', 'dokan-lite' ), number_format_i18n( $maximum_tags_select_length ) )
+ );
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
/** | |
* Maximum number of tags a vendor can add. | |
* | |
* @since 3.3.7 | |
* | |
* @args int -1 default to unlimited | |
*/ | |
$maximum_tags_select_length = apply_filters( 'dokan_product_tags_select_max_length', -1 ); | |
if ( $maximum_tags_select_length !== -1 && count( $tags ) > $maximum_tags_select_length ) { | |
// translators: %s: maximum tag length | |
return sprintf( __( 'You can only select %s tags', 'dokan-lite' ), number_format_i18n( $maximum_tags_select_length ) ); | |
} | |
// todo: add support for creating tags | |
return array_map( | |
function ( $tag ) { | |
if ( is_numeric( $tag ) ) { | |
return absint( $tag ); | |
} else { | |
return sanitize_text_field( $tag ); | |
} | |
}, (array) $tags | |
/** | |
* Maximum number of tags a vendor can add. | |
* | |
* @since 3.3.7 | |
* | |
* @args int -1 default to unlimited | |
*/ | |
$maximum_tags_select_length = apply_filters( 'dokan_product_tags_select_max_length', -1 ); | |
if ( $maximum_tags_select_length !== -1 && count( $tags ) > $maximum_tags_select_length ) { | |
// translators: %s: maximum tag length | |
return new \WP_Error( | |
'too_many_tags', | |
sprintf( __( 'You can only select %s tags', 'dokan-lite' ), number_format_i18n( $maximum_tags_select_length ) ) | |
); | |
} | |
// todo: add support for creating tags | |
return array_map( | |
function ( $tag ) { | |
if ( is_numeric( $tag ) ) { | |
return absint( $tag ); | |
} else { | |
return sanitize_text_field( $tag ); | |
} | |
}, (array) $tags |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (2)
templates/products/dokan-category-header-ui.php (1)
25-25
: Consider using more specific escaping functionWhile
wp_kses_post()
is secure, consider usingesc_html()
if$required_symbol
is meant to only contain plain text (like an asterisk *). If it's meant to contain HTML (like<span class="required">*</span>
), thenwp_kses_post()
is appropriate.- <?php echo wp_kses_post( $required_symbol ?? '' ); ?> + <?php echo esc_html( $required_symbol ?? '' ); ?>includes/Dashboard/Templates/Products.php (1)
373-375
: Enhance nonce verification failure handlingWhen nonce verification fails, it's good practice to inform the user about the issue. Consider adding an error message or redirecting the user to an error page to improve the user experience.
Apply this diff to add an error message upon nonce verification failure:
if ( ! isset( $_POST['dokan_edit_product_nonce'] ) || ! wp_verify_nonce( sanitize_key( $_POST['dokan_edit_product_nonce'] ), 'dokan_edit_product' ) ) { + self::$errors[] = __( 'Security verification failed. Please try again.', 'dokan-lite' ); return; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
includes/Dashboard/Templates/Products.php
(9 hunks)includes/ProductForm/Init.php
(1 hunks)templates/products/dokan-category-header-ui.php
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- includes/ProductForm/Init.php
🔇 Additional comments (4)
templates/products/dokan-category-header-ui.php (2)
23-26
: HTML structure improvement was previously suggested
A previous review already suggested improving the HTML structure by using the <label>
tag directly instead of wrapping it in a <div>
. That suggestion is still valid but unrelated to the current changes.
23-26
: Consider PHP version compatibility with null coalescing operator
The null coalescing operator (??
) requires PHP 7.0+. While most modern WordPress installations use PHP 7.0+, it's good practice to ensure compatibility or document the minimum PHP version requirement.
✅ Verification successful
PHP version compatibility is not a concern
The codebase already requires PHP 7.4 as specified in README.md, which fully supports the null coalescing operator (introduced in PHP 7.0). The usage is safe and aligned with the project's minimum requirements.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if minimum PHP version is specified in composer.json or readme
rg -i "require.*php.*[0-9]" composer.json README.md
Length of output: 87
includes/Dashboard/Templates/Products.php (2)
153-157
: Verify product creation permissions and error handling
While creating a new product, ensure that necessary permission checks are in place and handle any potential exceptions or errors that may occur during the product creation process.
Run the following script to confirm that product creation respects user permissions:
#!/bin/bash
# Description: Verify that only authorized users can create new products.
# Test: Search for permission checks before product creation.
rg 'new WC_Product_Simple' -A 10 | rg 'current_user_can\('
481-487
: Ensure proper sanitization of input data
While sanitizing the $file_names
, $file_urls
, and $file_hashes
, consider adding additional validation to ensure the data is safe and meets expected formats. This helps prevent potential security issues related to file handling.
Run the following script to check for any unsanitized inputs:
* | ||
* @var array | ||
*/ | ||
public static $errors = []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Avoid using static properties for instance-specific data
Using static properties like $errors
, $product_cat
, and $post_content
can lead to unexpected behavior in environments where multiple instances of the Products
class are used concurrently. This may cause data conflicts between users. Consider converting these static properties to instance properties to ensure data encapsulation and prevent potential conflicts.
Apply this diff to change static properties to instance properties:
-public static $errors = [];
-public static $product_cat;
-public static $post_content;
+public $errors = [];
+public $product_cat;
+public $post_content;
Also applies to: 38-38, 45-45
wp_set_object_terms( $product_id, $tags_ids, 'product_tag' ); | ||
} | ||
if ( ! dokan_is_product_author( $product_id ) ) { | ||
self::$errors[] = __( 'I swear this is not your product!', 'dokan-lite' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace informal error message with a user-friendly message
The error message 'I swear this is not your product!'
is unprofessional and may not be appropriate for users. Please replace it with a clearer and more formal message, such as 'You do not have permission to edit this product.'
Apply this diff to fix the error message:
- self::$errors[] = __( 'I swear this is not your product!', 'dokan-lite' );
+ self::$errors[] = __( 'You do not have permission to edit this product.', 'dokan-lite' );
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
self::$errors[] = __( 'I swear this is not your product!', 'dokan-lite' ); | |
self::$errors[] = __( 'You do not have permission to edit this product.', 'dokan-lite' ); |
if ( ! isset( $_GET['_dokan_edit_product_nonce'], $_GET['product_id'] ) || ! wp_verify_nonce( sanitize_key( $_GET['_dokan_edit_product_nonce'] ), 'dokan_edit_product_nonce' ) ) { | ||
dokan_get_template_part( | ||
'global/dokan-error', '', [ | ||
'deleted' => false, | ||
'message' => __( 'Are you cheating?', 'dokan-lite' ), | ||
] | ||
); | ||
|
||
return; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace informal error message with a user-friendly message
The error message 'Are you cheating?'
is informal and may confuse or offend users. Consider replacing it with a more professional and descriptive message, such as 'Invalid request.'
or 'Security verification failed.'
Apply this diff to fix the error message:
- 'message' => __( 'Are you cheating?', 'dokan-lite' ),
+ 'message' => __( 'Invalid request.', 'dokan-lite' ),
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
if ( ! isset( $_GET['_dokan_edit_product_nonce'], $_GET['product_id'] ) || ! wp_verify_nonce( sanitize_key( $_GET['_dokan_edit_product_nonce'] ), 'dokan_edit_product_nonce' ) ) { | |
dokan_get_template_part( | |
'global/dokan-error', '', [ | |
'deleted' => false, | |
'message' => __( 'Are you cheating?', 'dokan-lite' ), | |
] | |
); | |
return; | |
} | |
if ( ! isset( $_GET['_dokan_edit_product_nonce'], $_GET['product_id'] ) || ! wp_verify_nonce( sanitize_key( $_GET['_dokan_edit_product_nonce'] ), 'dokan_edit_product_nonce' ) ) { | |
dokan_get_template_part( | |
'global/dokan-error', '', [ | |
'deleted' => false, | |
'message' => __( 'Invalid request.', 'dokan-lite' ), | |
] | |
); | |
return; | |
} |
$file_urls = isset( $_POST['_wc_file_urls'] ) ? wp_unslash( $_POST['_wc_file_urls'] ) : []; // phpcs:ignore WordPress.Security.ValidatedSanitizedInput.InputNotSanitized | ||
$file_hashes = isset( $_POST['_wc_file_hashes'] ) ? wp_unslash( $_POST['_wc_file_hashes'] ) : []; // phpcs:ignore WordPress.Security.ValidatedSanitizedInput.InputNotSanitized | ||
$field_value = $field->sanitize( $file_names, $file_urls, $file_hashes ); //phpcs:ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Validate and sanitize file upload inputs
Ensure that the file URLs provided in $_POST['_wc_file_urls']
are properly validated and sanitized to prevent potential security risks such as XSS or file inclusion vulnerabilities.
Consider using esc_url_raw()
to sanitize URLs:
$field_value = $field->sanitize(
$file_names,
- $file_urls,
+ array_map( 'esc_url_raw', $file_urls ),
$file_hashes
);
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
$file_urls = isset( $_POST['_wc_file_urls'] ) ? wp_unslash( $_POST['_wc_file_urls'] ) : []; // phpcs:ignore WordPress.Security.ValidatedSanitizedInput.InputNotSanitized | |
$file_hashes = isset( $_POST['_wc_file_hashes'] ) ? wp_unslash( $_POST['_wc_file_hashes'] ) : []; // phpcs:ignore WordPress.Security.ValidatedSanitizedInput.InputNotSanitized | |
$field_value = $field->sanitize( $file_names, $file_urls, $file_hashes ); //phpcs:ignore | |
$file_urls = isset( $_POST['_wc_file_urls'] ) ? wp_unslash( $_POST['_wc_file_urls'] ) : []; // phpcs:ignore WordPress.Security.ValidatedSanitizedInput.InputNotSanitized | |
$file_hashes = isset( $_POST['_wc_file_hashes'] ) ? wp_unslash( $_POST['_wc_file_hashes'] ) : []; // phpcs:ignore WordPress.Security.ValidatedSanitizedInput.InputNotSanitized | |
$field_value = $field->sanitize( $file_names, array_map( 'esc_url_raw', $file_urls ), $file_hashes ); //phpcs:ignore |
…ory from assets class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
includes/ProductForm/Factory.php (2)
188-188
: Typo in documentation comment.There's a misspelling in the docblock comment on line 188. The word "registerd" should be corrected to "registered."
Apply this diff to fix the typo:
- * Returns registerd section with given section id. + * Returns registered section with given section id.
285-293
: Improve exception handling increate_item
method.In the
create_item
method, exceptions are caught broadly, which might obscure specific errors during item creation. Additionally, the exception class is not fully qualified, which could cause issues if there's a localException
class.Consider specifying the global namespace for the
Exception
class and handling specific exceptions if applicable:try { return apply_filters( 'dokan_product_form_item_created', new $class_name( $id, $args ), $type ); - } catch ( \Exception $e ) { + } catch ( \Exception $e ) { return new WP_Error( 'dokan_product_form_' . $type . '_class_creation', $e->getMessage() ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
includes/Assets.php
(8 hunks)includes/ProductForm/Factory.php
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- includes/Assets.php
esc_html__( 'You have attempted to register a duplicate form %1$s with WooCommerce Form: %2$s', 'dokan-lite' ), | ||
$type, | ||
'`' . $id . '`' | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incorrect product name in error message.
The error message references "WooCommerce Form" instead of "Dokan Product Form." This may cause confusion for developers using the Dokan plugin.
Update the error message to correctly reference "Dokan Product Form":
esc_html__( 'You have attempted to register a duplicate form %1$s with WooCommerce Form: %2$s', 'dokan-lite' ),
+ esc_html__( 'You have attempted to register a duplicate %1$s with Dokan Product Form: %2$s', 'dokan-lite' ),
Committable suggestion skipped: line range outside the PR's diff.
public static function add_section( $id, $args ) { | ||
if ( empty( $args['id'] ) ) { | ||
$args['id'] = $id; | ||
} | ||
|
||
$new_section = self::create_item( 'section', 'Section', $id, $args ); | ||
if ( is_wp_error( $new_section ) ) { | ||
return $new_section; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Validate required parameters in add_section
method.
Similarly, the add_section
method does not validate that the required argument title
is provided in the $args
array. Omitting this can cause issues when creating a new section.
Add validation to ensure the title
parameter is provided:
public static function add_section( $id, $args ) {
if ( empty( $args['id'] ) ) {
$args['id'] = $id;
}
+ if ( empty( $args['title'] ) ) {
+ return new WP_Error( 'dokan_product_form_missing_section_title', esc_html__( 'Section title is required.', 'dokan-lite' ) );
+ }
+
$new_section = self::create_item( 'section', 'Section', $id, $args );
if ( is_wp_error( $new_section ) ) {
return $new_section;
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
public static function add_section( $id, $args ) { | |
if ( empty( $args['id'] ) ) { | |
$args['id'] = $id; | |
} | |
$new_section = self::create_item( 'section', 'Section', $id, $args ); | |
if ( is_wp_error( $new_section ) ) { | |
return $new_section; | |
} | |
public static function add_section( $id, $args ) { | |
if ( empty( $args['id'] ) ) { | |
$args['id'] = $id; | |
} | |
if ( empty( $args['title'] ) ) { | |
return new WP_Error( 'dokan_product_form_missing_section_title', esc_html__( 'Section title is required.', 'dokan-lite' ) ); | |
} | |
$new_section = self::create_item( 'section', 'Section', $id, $args ); | |
if ( is_wp_error( $new_section ) ) { | |
return $new_section; | |
} |
public static function add_field( $id, $args ) { | ||
if ( empty( $args['id'] ) ) { | ||
$args['id'] = $id; | ||
} | ||
|
||
$new_field = self::create_item( 'field', 'Field', $id, $args ); | ||
if ( is_wp_error( $new_field ) ) { | ||
return $new_field; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Validate required parameters in add_field
method.
The add_field
method does not validate that required arguments such as title
and section
are provided in the $args
array. This may lead to errors if these parameters are missing when creating a new field.
Consider adding validation to ensure that required parameters are provided:
public static function add_field( $id, $args ) {
if ( empty( $args['id'] ) ) {
$args['id'] = $id;
}
+ if ( empty( $args['title'] ) ) {
+ return new WP_Error( 'dokan_product_form_missing_field_title', esc_html__( 'Field title is required.', 'dokan-lite' ) );
+ }
+
+ if ( empty( $args['section'] ) ) {
+ return new WP_Error( 'dokan_product_form_missing_field_section', esc_html__( 'Field section is required.', 'dokan-lite' ) );
+ }
+
$new_field = self::create_item( 'field', 'Field', $id, $args );
if ( is_wp_error( $new_field ) ) {
return $new_field;
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
public static function add_field( $id, $args ) { | |
if ( empty( $args['id'] ) ) { | |
$args['id'] = $id; | |
} | |
$new_field = self::create_item( 'field', 'Field', $id, $args ); | |
if ( is_wp_error( $new_field ) ) { | |
return $new_field; | |
} | |
public static function add_field( $id, $args ) { | |
if ( empty( $args['id'] ) ) { | |
$args['id'] = $id; | |
} | |
if ( empty( $args['title'] ) ) { | |
return new WP_Error( 'dokan_product_form_missing_field_title', esc_html__( 'Field title is required.', 'dokan-lite' ) ); | |
} | |
if ( empty( $args['section'] ) ) { | |
return new WP_Error( 'dokan_product_form_missing_field_section', esc_html__( 'Field section is required.', 'dokan-lite' ) ); | |
} | |
$new_field = self::create_item( 'field', 'Field', $id, $args ); | |
if ( is_wp_error( $new_field ) ) { | |
return $new_field; | |
} |
# Conflicts: # dokan.php # tests/pw/pages/productsPage.ts # tests/pw/pages/selectors.ts
… service provider
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (5)
dokan-class.php (1)
17-17
: Consider documenting the dashboard integration patternThe addition of the dashboard manager and template directory constant suggests an architectural shift towards better separation of concerns. Consider:
- Documenting the integration pattern between the dashboard manager and other components.
- Adding usage examples in the developer documentation.
- Creating migration guides if this changes existing template handling patterns.
Also applies to: 218-218
includes/Product/Hooks.php (1)
Line range hint
420-460
: Add null check and optimize array operations.The method needs some improvements for robustness and efficiency:
- Add null check for vendor object
- Simplify array operations
Consider this refactor:
public function product_review_notification_recipients( $emails, $comment_id ) { $comment = get_comment( $comment_id ); $product = wc_get_product( $comment->comment_post_ID ); if ( ! $product ) { return $emails; } - // Facilitate unsetting below without knowing the keys. - $filtered_emails = array_flip( $emails ); - $vendor = dokan_get_vendor_by_product( $product->get_id() ); + if ( ! $vendor ) { + return $emails; + } - if ( array_key_exists( $vendor->get_email(), $filtered_emails ) ) { - unset( $filtered_emails[ $vendor->get_email() ] ); - } + $filtered_emails = array_diff( $emails, [ $vendor->get_email() ] ); - // revert the array flip - $filtered_emails = array_flip( $filtered_emails ); - // get admin email $admin_email = get_option( 'admin_email' ); - if ( ! in_array( $admin_email, $filtered_emails, true ) ) { - $filtered_emails[] = $admin_email; - } + $filtered_emails = array_unique( [...$filtered_emails, $admin_email] ); return $filtered_emails; }includes/Dashboard/Templates/Products.php (1)
205-208
: Consider caching ProductFormFactory section instancesMultiple calls to
ProductFormFactory::get_section()
with the same section name could be optimized by caching the section instances. This would reduce unnecessary object creation and improve performance.+private static $cached_sections = []; public static function load_download_virtual_template( $post, $post_id ) { $product = wc_get_product( $post_id ); if ( ! $product ) { return; } - $section = ProductFormFactory::get_section( 'downloadable' ); + if ( ! isset( self::$cached_sections['downloadable'] ) ) { + self::$cached_sections['downloadable'] = ProductFormFactory::get_section( 'downloadable' ); + } + $section = self::$cached_sections['downloadable'];Also applies to: 237-240, 273-276, 303-306
tests/pw/utils/interfaces.ts (1)
Line range hint
126-131
: Remove duplicatedescription
properties in interfaces.Several interfaces have duplicate
description
properties which is redundant and may cause confusion:
simple
interface (lines 126 and 131)downloadable
interface (lines 140 and 145)variable
interface (lines 161 and 171)external
interface (lines 182 and 185)vendorSubscription
interface (lines 247 and 253)Remove the duplicate properties to maintain clean and unambiguous interfaces. For each interface, keep only one
description
property.interface simple { productType: string; productName: () => string; category: string; regularPrice: () => string; description: string; storeName: string; status: string; stockStatus: boolean; editProduct: string; - description: string; saveSuccessMessage: string; } interface downloadable { productType: string; productName: () => string; category: string; regularPrice: () => string; description: string; storeName: string; status: string; stockStatus: boolean; editProduct: string; - description: string; saveSuccessMessage: string; // ... rest of the interface } interface variable { productType: string; productName: () => string; category: string; regularPrice: () => string; description: string; storeName: string; status: string; stockStatus: boolean; attribute: string; attributeTerms: string[]; variations: { linkAllVariation: string; variableRegularPrice: string; }; - description: string; saveSuccessMessage: string; } interface external { productType: string; productName: () => string; productUrl: string; buttonText: string; category: string; regularPrice: () => string; description: string; storeName: string; status: string; - description: string; saveSuccessMessage: string; } interface vendorSubscription { productType: string; productName: () => string; category: string; regularPrice: () => string; description: string; numberOfProducts: string; packValidity: string; advertisementSlot: string; expireAfterDays: string; storeName: string; - description: string; status: string; }Also applies to: 140-145, 161-171, 182-185, 247-253
includes/functions.php (1)
702-716
: LGTM! Consider adding ARIA attributes for accessibilityThe addition of the description tooltip is well implemented with proper escaping. For better accessibility, consider adding ARIA attributes.
<i class="fas fa-question-circle tips" aria-hidden="true" + role="tooltip" + aria-label="<?php echo esc_attr( $desc ); ?>" data-title="<?php echo esc_attr( $desc ); ?>"> </i>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
assets/src/less/products.less
(3 hunks)dokan-class.php
(2 hunks)includes/Assets.php
(8 hunks)includes/Dashboard/Templates/Products.php
(9 hunks)includes/DependencyManagement/Providers/CommonServiceProvider.php
(1 hunks)includes/Product/Hooks.php
(4 hunks)includes/functions.php
(2 hunks)tests/pw/pages/settingsPage.ts
(0 hunks)tests/pw/utils/dbData.ts
(0 hunks)tests/pw/utils/interfaces.ts
(7 hunks)
💤 Files with no reviewable changes (2)
- tests/pw/pages/settingsPage.ts
- tests/pw/utils/dbData.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- assets/src/less/products.less
- includes/Assets.php
🔇 Additional comments (10)
includes/DependencyManagement/Providers/CommonServiceProvider.php (2)
69-71
: LGTM! Service registration follows established patterns.
The registration of ProductForm\Init
service maintains consistency with other service registrations in the provider, properly utilizing singleton pattern via addShared()
and tagged appropriately.
69-71
: Verify ProductForm\Init implementation.
Let's ensure the ProductForm\Init
class exists and is properly implemented.
✅ Verification successful
Let me check the actual implementation of the Init class to verify it properly.
The ProductForm\Init class is properly implemented and verified.
The class exists at includes/ProductForm/Init.php
and is well-structured with:
- Proper namespace
WeDevs\Dokan\ProductForm
- Constructor that hooks into WordPress
init
action - Complete implementation of form field initialization methods for:
- General product fields
- Inventory management
- Downloadable options
- Other product settings
The class is correctly registered in the dependency container and its implementation matches its intended purpose of initializing product form fields.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and implementation of ProductForm\Init class
# Check if the Init class file exists
fd -e php "Init.php" includes/ProductForm
# Check the class implementation
ast-grep --pattern 'class Init {
$$$
}'
# Look for any usage of this class
rg -l "ProductForm.Init" --type php
Length of output: 217
Script:
#!/bin/bash
# Get the implementation of Init class
cat includes/ProductForm/Init.php
Length of output: 29629
dokan-class.php (1)
17-17
: LGTM: Well-documented dashboard property addition
The new dashboard property is properly documented with the correct namespace and follows the existing pattern of property documentation in the class.
includes/Product/Hooks.php (3)
8-8
: LGTM! Good security practice.
The addition of the direct access prevention check follows WordPress security best practices.
35-35
: LGTM! Hook registration is correct.
The filter hook is properly registered with appropriate priority and follows WordPress naming conventions.
369-382
: Previous review comment is still applicable.
The suggested refactor from the previous review would improve the code's clarity and efficiency.
includes/Dashboard/Templates/Products.php (2)
153-156
: Consider adding status transition handling for auto-draft products
When creating a new product with 'auto-draft' status, consider implementing cleanup logic for abandoned drafts to prevent database clutter. Also, verify that the status transitions correctly when the product is actually saved.
57-70
: Verify hook priorities for potential race conditions
The action hooks have different priorities (11, 10, 5, 85) which could potentially lead to execution order issues. Consider documenting the reasoning behind these specific priority numbers or standardizing them if there's no specific requirement.
✅ Verification successful
Let me gather more context about the template loading sequence by checking the implementation of these methods.
Let me try a different approach using ripgrep to find these methods.
Let me check if there are any template files that these methods load to understand their dependencies.
Based on the analysis of the template loading sequence and their implementations, I can now provide a final response:
Hook priorities are correctly set based on template dependencies
The different priority numbers in the action hooks are intentional and necessary:
load_product_edit_template
(11): Main template loader that sets up the product editing environmentload_download_virtual_template
(10): Loads first section after titleload_inventory_template
(5): Loads before downloadable template as inventory data is prerequisiteload_downloadable_template
(10): Depends on inventory being loaded firstload_others_template
(85): Intentionally loads last after all core product data sections
Each template section loads independently with proper product validation, making the sequence safe from race conditions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any other plugins hooking into the same actions with competing priorities
rg -A 2 "add_action\(\s*'dokan_render_product_edit_template'|add_action\(\s*'dokan_product_edit_after_title'|add_action\(\s*'dokan_product_edit_after_main'|add_action\(\s*'dokan_product_edit_after_inventory_variants'"
Length of output: 1129
Script:
#!/bin/bash
# Check the implementation of template loading methods to understand the dependency order
ast-grep --pattern 'function load_product_edit_template($$$) { $$$ }'
ast-grep --pattern 'function load_download_virtual_template($$$) { $$$ }'
ast-grep --pattern 'function load_inventory_template($$$) { $$$ }'
ast-grep --pattern 'function load_downloadable_template($$$) { $$$ }'
ast-grep --pattern 'function load_others_template($$$) { $$$ }'
Length of output: 354
Script:
#!/bin/bash
# Search for the template loading methods implementations
rg -A 5 "public (static )?function (load_product_edit_template|load_download_virtual_template|load_inventory_template|load_downloadable_template|load_others_template)"
Length of output: 2424
Script:
#!/bin/bash
# Search for template loading calls within these methods
rg -B 2 -A 10 "dokan_get_template(_part)?\(" includes/Dashboard/Templates/Products.php
Length of output: 3270
tests/pw/utils/interfaces.ts (1)
126-126
: LGTM: Description property additions are consistent.
The addition of the description
property across product-related interfaces is consistent and well-structured. The changes align well with the PR objectives of enhancing product form handling.
Also applies to: 140-140, 161-161, 182-182, 247-247
includes/functions.php (1)
652-652
: Improved required attribute validation
The change from isset()
to ! empty()
provides more robust validation by ensuring the required attribute has a truthy value, not just that it exists.
@@ -214,6 +215,7 @@ public function define_constants() { | |||
defined( 'DOKAN_PLUGIN_VERSION' ) || define( 'DOKAN_PLUGIN_VERSION', $this->version ); | |||
defined( 'DOKAN_DIR' ) || define( 'DOKAN_DIR', __DIR__ ); | |||
defined( 'DOKAN_INC_DIR' ) || define( 'DOKAN_INC_DIR', __DIR__ . '/includes' ); | |||
defined( 'DOKAN_TEMPLATE_DIR' ) || define( 'DOKAN_TEMPLATE_DIR', __DIR__ . '/templates' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codebase verification
Template directory constant needs to be used consistently
The verification reveals inconsistent template path handling across the codebase:
- Most template loading is handled through functions like
dokan_locate_template()
anddokan_get_template()
which use dynamic path resolution - Email templates use
DOKAN_DIR . '/templates/'
- Only one direct usage of
DOKAN_TEMPLATE_DIR
found intemplates/products/edit/edit-product-single.php
For consistency and maintainability:
- Replace
DOKAN_DIR . '/templates/'
usage in email classes withDOKAN_TEMPLATE_DIR
- Consider updating the direct include in
edit-product-single.php
to use template loading functions instead
🔗 Analysis chain
Verify template directory constant usage
The constant definition looks good, but let's ensure it's being used consistently across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for existing usages of hardcoded template paths that should use DOKAN_TEMPLATE_DIR
# Search for hardcoded template paths
rg -g '*.php' "(?i)(templates?.*directory|template.*path|template.*dir)"
# Search for current usage of the new constant
rg "DOKAN_TEMPLATE_DIR"
Length of output: 6453
self::$errors[] = __( 'No product id is set!', 'dokan-lite' ); | ||
|
||
self::$errors = apply_filters( 'dokan_can_add_product', $errors ); | ||
|
||
if ( ! self::$errors ) { | ||
$timenow = dokan_current_datetime()->setTimezone( new \DateTimeZone( 'UTC' ) ); | ||
$product_status = dokan_get_default_product_status( dokan_get_current_user_id() ); | ||
$post_data = apply_filters( | ||
'dokan_insert_product_post_data', [ | ||
'post_type' => 'product', | ||
'post_status' => $product_status, | ||
'post_title' => $post_title, | ||
'post_content' => $post_content, | ||
'post_excerpt' => $post_excerpt, | ||
'post_date_gmt' => $timenow->format( 'Y-m-d H:i:s' ), | ||
'post_modified_gmt' => $timenow->format( 'Y-m-d H:i:s' ), | ||
] | ||
); | ||
|
||
$product_id = wp_insert_post( $post_data ); | ||
|
||
if ( $product_id ) { | ||
// set images | ||
if ( $featured_image ) { | ||
set_post_thumbnail( $product_id, $featured_image ); | ||
} | ||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Ensure all error messages are translatable
Some error messages are not wrapped in translation functions. Ensure all user-facing messages are translatable for proper localization.
-self::$errors[] = 'No product id is set!';
+self::$errors[] = __( 'No product id is set!', 'dokan-lite' );
-self::$errors[] = 'No product found with given product id!';
+self::$errors[] = __( 'No product found with given product id!', 'dokan-lite' );
Also applies to: 399-402
// Skip the "Minimum Quantity" and "Discount %" fields if "Enable Bulk Discount" field disabled. | ||
if ( in_array( $field->get_id(), [ '_lot_discount_quantity', '_lot_discount_amount' ], true ) && isset( $_POST['_is_lot_discount'] ) && 'no' === $_POST['_is_lot_discount'] ) { | ||
continue; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add validation for discount percentage range
When handling bulk discounts, add validation to ensure the discount percentage is within a reasonable range (e.g., 0-100%). Currently, there's no validation for the discount amount.
if ( in_array( $field->get_id(), [ '_lot_discount_quantity', '_lot_discount_amount' ], true ) && isset( $_POST['_is_lot_discount'] ) && 'no' === $_POST['_is_lot_discount'] ) {
continue;
}
+// Add validation for discount percentage
+if ( '_lot_discount_amount' === $field->get_id() && isset( $_POST['_is_lot_discount'] ) && 'yes' === $_POST['_is_lot_discount'] ) {
+ $discount = floatval( $_POST['_lot_discount_amount'] );
+ if ( $discount < 0 || $discount > 100 ) {
+ self::$errors['_lot_discount_amount'] = __( 'Discount percentage must be between 0 and 100.', 'dokan-lite' );
+ continue;
+ }
+}
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
// Skip the "Minimum Quantity" and "Discount %" fields if "Enable Bulk Discount" field disabled. | |
if ( in_array( $field->get_id(), [ '_lot_discount_quantity', '_lot_discount_amount' ], true ) && isset( $_POST['_is_lot_discount'] ) && 'no' === $_POST['_is_lot_discount'] ) { | |
continue; | |
} | |
// Skip the "Minimum Quantity" and "Discount %" fields if "Enable Bulk Discount" field disabled. | |
if ( in_array( $field->get_id(), [ '_lot_discount_quantity', '_lot_discount_amount' ], true ) && isset( $_POST['_is_lot_discount'] ) && 'no' === $_POST['_is_lot_discount'] ) { | |
continue; | |
} | |
// Add validation for discount percentage | |
if ( '_lot_discount_amount' === $field->get_id() && isset( $_POST['_is_lot_discount'] ) && 'yes' === $_POST['_is_lot_discount'] ) { | |
$discount = floatval( $_POST['_lot_discount_amount'] ); | |
if ( $discount < 0 || $discount > 100 ) { | |
self::$errors['_lot_discount_amount'] = __( 'Discount percentage must be between 0 and 100.', 'dokan-lite' ); | |
continue; | |
} | |
} |
# Conflicts: # includes/ProductCategory/Categories.php # templates/products/edit-product-single.php
…lving merge conflict
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (4)
src/admin/pages/Settings.vue (2)
52-54
: Add prop validation for action button properties.Consider adding prop validation to ensure
action_button
andaction_button_label
are properly defined in the section object.export default { // ... + props: { + section: { + type: Object, + required: true, + validator: function(obj) { + return typeof obj.action_button === 'string' && + typeof obj.action_button_label === 'string'; + } + } + }, // ... }
71-71
: Optimize prop passing to Fields component.Passing the entire
settingFields
object could trigger unnecessary re-renders. Consider passing only the required subset of fields for the current section.-:setting-fields='settingFields' +:setting-fields='settingFields[index]'src/admin/components/Fields.vue (1)
498-499
: Add validation for custom field components.The custom field components receive new props but lack validation. Consider adding prop validation in the component registration process.
Add validation when registering custom components through the
getDokanCustomFieldComponents
filter:// Example validation in custom component registration dokan.hooks.addFilter('getDokanCustomFieldComponents', 'dokan', function(components) { return components.map(component => { // Ensure component has prop validation if (!component.props) { component.props = { allSettingsValues: { type: Object, required: true }, settingFields: { type: Object, required: true } }; } return component; }); });templates/products/edit/edit-product-single.php (1)
15-15
: Avoid using global variables and disabling coding standardsUsing
global $post;
and disablingWordPress.WP.GlobalVariablesOverride.Prohibited
is discouraged. Refactor the code to pass the$post
variable explicitly without relying on the global scope to enhance code clarity and maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
assets/src/less/products.less
(3 hunks)dokan-class.php
(2 hunks)src/admin/components/Fields.vue
(2 hunks)src/admin/pages/Settings.vue
(6 hunks)templates/products/dokan-category-header-ui.php
(1 hunks)templates/products/edit/edit-product-single.php
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- assets/src/less/products.less
- dokan-class.php
- templates/products/dokan-category-header-ui.php
🔇 Additional comments (2)
src/admin/pages/Settings.vue (1)
Line range hint 763-835
: LGTM! Styles are well-organized and consistent.
The new styles for the settings header and action link follow the existing patterns and maintain visual consistency.
templates/products/edit/edit-product-single.php (1)
170-170
: 🛠️ Refactor suggestion
Use <button>
element instead of <a>
for dismissing alerts
Replace the <a>
tag with a <button>
element for the close button to improve accessibility and semantics, especially since the anchor lacks an href
attribute.
Apply this diff to fix the issue:
-<a class="dokan-close" data-dismiss="alert">×</a>
+<button type="button" class="dokan-close" data-dismiss="alert">×</button>
Likely invalid or redundant comment.
handleAction( sectionId ) { | ||
if ( 'dokan_form_manager' === sectionId ) { | ||
this.resetAllFields( sectionId ); | ||
} | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Decouple section-specific logic from handleAction method.
The method is tightly coupled to a specific section ID. Consider implementing a more flexible approach using a strategy pattern or action map.
-handleAction( sectionId ) {
- if ( 'dokan_form_manager' === sectionId ) {
- this.resetAllFields( sectionId );
- }
-},
+const ACTION_HANDLERS = {
+ 'dokan_form_manager': function(sectionId) {
+ this.resetAllFields(sectionId);
+ }
+};
+
+handleAction(sectionId) {
+ const handler = ACTION_HANDLERS[sectionId];
+ if (handler) {
+ handler.call(this, sectionId);
+ }
+},
Committable suggestion skipped: line range outside the PR's diff.
resetAllFields( sectionId ) { | ||
let settingFields = Object.entries( this.settingFields[sectionId] ); | ||
let clonedSectionSettingValues = JSON.parse( JSON.stringify( this.settingValues[sectionId] ) ); | ||
|
||
settingFields.forEach( ( key, value ) => { | ||
let blockId = key[0]; | ||
|
||
if ( blockId ) { | ||
let sectionValue = key[1]; | ||
for (const [FieldId, fieldData] of Object.entries(sectionValue.fields)) { | ||
clonedSectionSettingValues[blockId]['fields'][FieldId]['visibility'] = fieldData.default.visibility; | ||
clonedSectionSettingValues[blockId]['fields'][FieldId]['required'] = fieldData.default.required; | ||
} | ||
} | ||
} ); | ||
|
||
this.settingValues[sectionId] = clonedSectionSettingValues; | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Improve resetAllFields method implementation.
The current implementation has several potential issues:
- Uses JSON parse/stringify for cloning which doesn't handle special values
- Nested loops could impact performance
- No error handling for malformed data
resetAllFields(sectionId) {
+ try {
- let settingFields = Object.entries(this.settingFields[sectionId]);
- let clonedSectionSettingValues = JSON.parse(JSON.stringify(this.settingValues[sectionId]));
+ const clonedSectionSettingValues = structuredClone(this.settingValues[sectionId]);
+ const settingFields = this.settingFields[sectionId];
- settingFields.forEach((key, value) => {
- let blockId = key[0];
+ Object.entries(settingFields).forEach(([blockId, sectionValue]) => {
+ if (!blockId || !sectionValue?.fields) return;
- if (blockId) {
- let sectionValue = key[1];
- for (const [FieldId, fieldData] of Object.entries(sectionValue.fields)) {
- clonedSectionSettingValues[blockId]['fields'][FieldId]['visibility'] = fieldData.default.visibility;
- clonedSectionSettingValues[blockId]['fields'][FieldId]['required'] = fieldData.default.required;
- }
+ Object.entries(sectionValue.fields).forEach(([fieldId, fieldData]) => {
+ if (!fieldData?.default) return;
+
+ clonedSectionSettingValues[blockId].fields[fieldId] = {
+ ...clonedSectionSettingValues[blockId].fields[fieldId],
+ visibility: fieldData.default.visibility,
+ required: fieldData.default.required
+ };
+ });
+ });
- });
this.settingValues[sectionId] = clonedSectionSettingValues;
+ } catch (error) {
+ console.error('Failed to reset fields:', error);
+ // Consider showing user-friendly error message
}
},
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
resetAllFields( sectionId ) { | |
let settingFields = Object.entries( this.settingFields[sectionId] ); | |
let clonedSectionSettingValues = JSON.parse( JSON.stringify( this.settingValues[sectionId] ) ); | |
settingFields.forEach( ( key, value ) => { | |
let blockId = key[0]; | |
if ( blockId ) { | |
let sectionValue = key[1]; | |
for (const [FieldId, fieldData] of Object.entries(sectionValue.fields)) { | |
clonedSectionSettingValues[blockId]['fields'][FieldId]['visibility'] = fieldData.default.visibility; | |
clonedSectionSettingValues[blockId]['fields'][FieldId]['required'] = fieldData.default.required; | |
} | |
} | |
} ); | |
this.settingValues[sectionId] = clonedSectionSettingValues; | |
}, | |
resetAllFields( sectionId ) { | |
try { | |
const clonedSectionSettingValues = structuredClone(this.settingValues[sectionId]); | |
const settingFields = this.settingFields[sectionId]; | |
Object.entries(settingFields).forEach(([blockId, sectionValue]) => { | |
if (!blockId || !sectionValue?.fields) return; | |
Object.entries(sectionValue.fields).forEach(([fieldId, fieldData]) => { | |
if (!fieldData?.default) return; | |
clonedSectionSettingValues[blockId].fields[fieldId] = { | |
...clonedSectionSettingValues[blockId].fields[fieldId], | |
visibility: fieldData.default.visibility, | |
required: fieldData.default.required | |
}; | |
}); | |
}); | |
this.settingValues[sectionId] = clonedSectionSettingValues; | |
} catch (error) { | |
console.error('Failed to reset fields:', error); | |
// Consider showing user-friendly error message | |
} | |
}, |
@@ -535,7 +537,7 @@ | |||
WithdrawCharges | |||
}, | |||
|
|||
props: ['id', 'fieldData', 'sectionId', 'fieldValue', 'allSettingsValues', 'errors', 'toggleLoadingState', 'validationErrors', 'dokanAssetsUrl'], | |||
props: ['id', 'fieldData', 'sectionId', 'fieldValue', 'allSettingsValues', 'errors', 'toggleLoadingState', 'validationErrors', 'dokanAssetsUrl', 'settingFields'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Add prop validation and documentation.
The new settingFields
prop should include type validation and documentation to improve maintainability and prevent runtime errors.
-props: ['id', 'fieldData', 'sectionId', 'fieldValue', 'allSettingsValues', 'errors', 'toggleLoadingState', 'validationErrors', 'dokanAssetsUrl', 'settingFields'],
+props: {
+ id: { type: String, required: true },
+ fieldData: { type: Object, required: true },
+ sectionId: { type: String, required: true },
+ fieldValue: { type: Object, required: true },
+ allSettingsValues: { type: Object, required: true },
+ errors: { type: Array, default: () => [] },
+ toggleLoadingState: { type: Function, required: true },
+ validationErrors: { type: Array, default: () => [] },
+ dokanAssetsUrl: { type: String, required: true },
+ settingFields: {
+ type: Object,
+ required: true,
+ description: 'Configuration object containing all available setting fields'
+ }
+}
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
props: ['id', 'fieldData', 'sectionId', 'fieldValue', 'allSettingsValues', 'errors', 'toggleLoadingState', 'validationErrors', 'dokanAssetsUrl', 'settingFields'], | |
props: { | |
id: { type: String, required: true }, | |
fieldData: { type: Object, required: true }, | |
sectionId: { type: String, required: true }, | |
fieldValue: { type: Object, required: true }, | |
allSettingsValues: { type: Object, required: true }, | |
errors: { type: Array, default: () => [] }, | |
toggleLoadingState: { type: Function, required: true }, | |
validationErrors: { type: Array, default: () => [] }, | |
dokanAssetsUrl: { type: String, required: true }, | |
settingFields: { | |
type: Object, | |
required: true, | |
description: 'Configuration object containing all available setting fields' | |
} | |
} |
|
||
<?php if ( $product->get_status() === 'publish' ) : ?> | ||
<span class="dokan-right"> | ||
<a class="dokan-btn dokan-btn-theme dokan-btn-sm" href="<?php echo esc_url( $product->get_permalink() ); ?>" target="_blank"><?php esc_html_e( 'View Product', 'dokan-lite' ); ?></a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add rel="noopener noreferrer"
when using target="_blank
Including rel="noopener noreferrer"
on links with target="_blank"
prevents the new page from accessing the original window object via window.opener
, enhancing security against phishing and tabnabbing attacks.
Apply this diff to fix the issue:
-<a class="dokan-btn dokan-btn-theme dokan-btn-sm" href="<?php echo esc_url( $product->get_permalink() ); ?>" target="_blank"><?php esc_html_e( 'View Product', 'dokan-lite' ); ?></a>
+<a class="dokan-btn dokan-btn-theme dokan-btn-sm" href="<?php echo esc_url( $product->get_permalink() ); ?>" target="_blank" rel="noopener noreferrer"><?php esc_html_e( 'View Product', 'dokan-lite' ); ?></a>
-<a href="<?php echo esc_url( $product->get_permalink() ); ?>" target="_blank"><?php esc_html_e( 'View Product →', 'dokan-lite' ); ?></a>
+<a href="<?php echo esc_url( $product->get_permalink() ); ?>" target="_blank" rel="noopener noreferrer"><?php esc_html_e( 'View Product →', 'dokan-lite' ); ?></a>
Also applies to: 182-182
// we are using require_once intentionally | ||
include DOKAN_TEMPLATE_DIR . '/products/edit/sections/general.php'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix inconsistency between comment and code regarding file inclusion
The comment mentions using require_once
intentionally, but the code uses include
. To ensure the file is included only once and prevent potential issues, replace include
with require_once
.
Apply this diff to fix the issue:
-// we are using require_once intentionally
-include DOKAN_TEMPLATE_DIR . '/products/edit/sections/general.php';
+// we are using require_once intentionally
+require_once DOKAN_TEMPLATE_DIR . '/products/edit/sections/general.php';
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
// we are using require_once intentionally | |
include DOKAN_TEMPLATE_DIR . '/products/edit/sections/general.php'; | |
// we are using require_once intentionally | |
require_once DOKAN_TEMPLATE_DIR . '/products/edit/sections/general.php'; |
} | ||
|
||
// while calling from theme, we need to check if the product id is passed or not | ||
$post_id = isset( $_GET['product_id'] ) ? intval( wp_unslash( $_GET['product_id'] ) ) : $post->ID; //phpcs:ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use absint
instead of intval
for sanitizing numeric input
Using absint
ensures you get a positive integer and aligns with WordPress coding standards for sanitizing user input.
Apply this diff to fix the issue:
-$post_id = isset( $_GET['product_id'] ) ? intval( wp_unslash( $_GET['product_id'] ) ) : $post->ID; //phpcs:ignore
+$post_id = isset( $_GET['product_id'] ) ? absint( wp_unslash( $_GET['product_id'] ) ) : $post->ID; //phpcs:ignore
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
$post_id = isset( $_GET['product_id'] ) ? intval( wp_unslash( $_GET['product_id'] ) ) : $post->ID; //phpcs:ignore | |
$post_id = isset( $_GET['product_id'] ) ? absint( wp_unslash( $_GET['product_id'] ) ) : $post->ID; //phpcs:ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (5)
templates/products/edit/sections/general.php (5)
74-96
: Simplify handling of single product type optionWhen there's only one product type option available (in this case, 'simple'), you're currently using a hidden input field. This can be simplified for better code clarity.
Instead of using an
if...else
block, you can set the default value directly or consider removing the product type selection when only one option is available.
37-66
: Handle product slug editing consistentlyThe current implementation includes hidden fields for
editable-post-name
andedited-post-name
but the slug editing functionality isn't fully implemented.Ensure that the product slug can be edited if required, or remove the unused fields to prevent confusion.
392-456
: Improve accessibility for text editorsThe labels for the short description and description fields are correctly associated, but consider enhancing the accessibility of the
wp_editor
instances.Add
aria-label
attributes or ensure the editors are fully accessible to users with assistive technologies.
327-345
: Provide alt text for featured imagesThe featured image markup lacks
alt
attributes, which can affect accessibility and SEO.Add meaningful
alt
text to the<img>
tags to improve accessibility.<?php else : ?> - <img height="" width="" src="" alt=""> + <img height="" width="" src="" alt="<?php esc_attr_e( 'Featured image', 'dokan-lite' ); ?>"> <?php endif; ?>
255-261
: Optimize category selection template inclusionYou're including a template part for the category selection. Ensure that the data passed to the template is sufficient and sanitized.
Double-check the
Helper::get_saved_products_category()
function to confirm it returns the expected data.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
templates/products/edit/sections/general.php
(1 hunks)
🔇 Additional comments (2)
templates/products/edit/sections/general.php (2)
103-135
: Consistent use of currency symbols
Ensure that the currency symbol is consistently displayed across all price input fields.
The implementation correctly uses get_woocommerce_currency_symbol()
, and the currency symbol is displayed as expected.
Also applies to: 138-185
163-185
: Check sale schedule toggle functionality
The toggling between "Schedule" and "Cancel" links for sale scheduling might be confusing if not properly handled.
Ensure that the JavaScript handling the show/hide functionality of the sale schedule fields works correctly.
Run the following script to confirm that the event listeners are properly set:
✅ Verification successful
Let me analyze the JavaScript handling of sale scheduling with these broader search patterns.
Let me analyze the JavaScript handling of sale scheduling with these more targeted search patterns.
From analyzing the code, I found that the sale schedule functionality is properly implemented in the JavaScript code. The relevant code shows:
- The sale schedule toggle functionality is handled by the
saleSchedule
andcancelSchedule
methods - The toggle behavior is properly bound to click events on
.sale_schedule
and.cancel_sale_schedule
elements - The methods handle showing/hiding the date fields and clearing values appropriately
Sale schedule toggle functionality works as expected
The JavaScript implementation shows that clicking the "Schedule" link will:
- Hide the schedule link
- Show the cancel link
- Show the date fields
And clicking "Cancel" will:
- Hide the cancel link
- Show the schedule link
- Hide and clear the date fields
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the sale schedule toggle links have correct event handlers.
# Test: Search for JavaScript handling 'sale_schedule' and 'cancel_sale_schedule' classes.
rg -A 5 '$(document).on\(' assets/js/ | rg 'sale_schedule|cancel_sale_schedule'
Length of output: 81
Script:
#!/bin/bash
# Let's try a broader search for sale schedule related JavaScript code
rg -i "sale.*schedule" assets/js/
# Also search for any click handlers in JavaScript files
rg "\.on\('click'" assets/js/
# Search for any files that might handle product scheduling
fd -e js . assets/js/ -x cat {} | rg -i "schedule|sale.*price"
Length of output: 125766
Script:
#!/bin/bash
# Let's search for JavaScript files specifically in the product-related directories
fd -e js . assets/js/dokan/product -x cat {} | rg -i "sale.*schedule|discount_schedule"
# Also search for any event handlers related to sale price scheduling
rg -i "sale.*schedule|discount_schedule" assets/js/dokan/product
# Let's also check the main product script
rg -i "sale.*schedule|discount_schedule" assets/js/dokan.js
Length of output: 68107
<label for="<?php echo esc_attr( $regular_price->get_name() ); ?>" class="form-label"> | ||
<?php echo esc_html( $regular_price->get_title() ); ?> | ||
<?php $regular_price->print_required_symbol(); ?> | ||
<i | ||
class="fas fa-question-circle tips <?php echo empty( $regular_price->get_help_content() ) ? 'dokan-hide' : ''; ?>" | ||
aria-hidden="true" | ||
data-title="<?php echo esc_attr( $regular_price->get_help_content() ); ?>"> | ||
</i> | ||
</label> | ||
<div class="dokan-input-group"> | ||
<span class="dokan-input-group-addon"> | ||
<?php echo esc_html( get_woocommerce_currency_symbol() ); ?> | ||
</span> | ||
<?php | ||
dokan_post_input_box( | ||
$product->get_id(), | ||
$regular_price->get_name(), | ||
[ | ||
'value' => $regular_price->get_value( $product ), | ||
'class' => 'dokan-product-regular-price', | ||
'placeholder' => $regular_price->get_placeholder(), | ||
'required' => $regular_price->is_required(), | ||
], | ||
'price' | ||
); | ||
?> | ||
</div> | ||
</div> | ||
<?php endif; ?> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Ensure proper validation for sale price and regular price
It's important to validate that the sale price is not higher than the regular price, as this could cause confusion or incorrect pricing.
Consider adding a validation check to ensure that the sale price is less than the regular price. You can implement this by adding a custom JavaScript validation or server-side validation during form submission.
Also applies to: 151-185
$product_images = is_array( $product_images ) ? $product_images : explode( ',', $product_images ); | ||
|
||
if ( $product_images ) : | ||
foreach ( $product_images as $image_id ) : | ||
if ( empty( $image_id ) ) : | ||
continue; | ||
endif; | ||
|
||
$attachment_image = wp_get_attachment_image_src( $image_id, 'thumbnail' ); | ||
?> | ||
<li class="image" data-attachment_id="<?php echo esc_attr( $image_id ); ?>"> | ||
<img src="<?php echo esc_url( $attachment_image[0] ); ?>" alt=""> | ||
<a href="#" class="action-delete" title="<?php esc_attr_e( 'Delete image', 'dokan-lite' ); ?>">×</a> | ||
</li> | ||
<?php | ||
endforeach; | ||
endif; | ||
?> | ||
<li class="add-image add-product-images tips" data-title="<?php esc_html_e( 'Add gallery image', 'dokan-lite' ); ?>"> | ||
<a href="#" class="add-product-images"><i class="fas fa-plus" aria-hidden="true"></i></a> | ||
</li> | ||
</ul> | ||
|
||
<input type="hidden" id="product_image_gallery" name="<?php echo esc_attr( $gallery_images->get_name() ); ?>" value="<?php echo esc_attr( implode( ',', $product_images ) ); ?>"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sanitize and validate gallery image IDs
When handling the gallery images, ensure that the image IDs are properly sanitized to prevent security issues.
Implement sanitization when processing the $product_images
array to prevent potential security vulnerabilities.
<label for="<?php echo esc_attr( $tags->get_name() ); ?>" class="form-label"> | ||
<?php echo $tags->get_title(); ?> | ||
<?php $tags->print_required_symbol(); ?> | ||
<i | ||
class="fas fa-question-circle tips <?php echo empty( $tags->get_help_content() ) ? 'dokan-hide' : ''; ?>" | ||
aria-hidden="true" | ||
data-title="<?php echo esc_attr( $tags->get_help_content() ); ?>"> | ||
</i> | ||
</label> | ||
<select multiple="multiple" id="<?php echo esc_attr( $tags->get_name() ); ?>" name="<?php echo esc_attr( $tags->get_name() ); ?>" class="product_tag_search dokan-form-control" data-placeholder="<?php echo esc_attr( $tags->get_placeholder() ); ?>" <?php echo $tags->is_required() ? 'required' : ''; ?>> | ||
<?php if ( ! empty( $terms ) ) : ?> | ||
<?php | ||
foreach ( $terms as $tax_term ) : | ||
$tax_term = get_term_by( 'id', $tax_term, 'product_tag' ); | ||
if ( is_wp_error( $tax_term ) ) { | ||
continue; | ||
} | ||
?> | ||
<option value="<?php echo esc_attr( $tax_term->term_id ); ?>" selected="selected"><?php echo esc_html( $tax_term->name ); ?></option> | ||
<?php endforeach ?> | ||
<?php endif ?> | ||
</select> | ||
</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Escape output in tags selection
When outputting the tag terms, make sure all outputs are properly escaped to prevent XSS vulnerabilities.
Apply proper escaping functions like esc_attr()
and esc_html()
where necessary.
type="text" | ||
name="<?php echo esc_attr( $date_on_sale_from->get_name() ); ?>" | ||
class="dokan-form-control dokan-start-date" | ||
value="<?php echo esc_attr( $sale_price_dates_from ); ?>" | ||
maxlength="10" | ||
pattern="[0-9]{4}-(0[1-9]|1[012])-(0[1-9]|1[0-9]|2[0-9]|3[01])" | ||
placeholder="<?php echo esc_attr( $date_on_sale_from->get_placeholder() ); ?>" | ||
<?php echo $date_on_sale_from->is_required() ? 'required' : ''; ?>> | ||
</div> | ||
</div> | ||
|
||
<div class="dokan-form-group content-half-part to"> | ||
<div class="dokan-input-group"> | ||
<span class="dokan-input-group-addon"> | ||
<?php echo esc_html( $date_on_sale_to->get_title() ); ?> | ||
<?php $date_on_sale_to->print_required_symbol(); ?> | ||
<i | ||
class="fas fa-question-circle tips <?php echo empty( $date_on_sale_to->get_help_content() ) ? 'dokan-hide' : ''; ?>" | ||
aria-hidden="true" | ||
data-title="<?php echo esc_attr( $date_on_sale_to->get_help_content() ); ?>"> | ||
</i> | ||
</span> | ||
<input | ||
type="text" | ||
name="<?php echo esc_attr( $date_on_sale_to->get_name() ); ?>" | ||
class="dokan-form-control dokan-end-date" | ||
value="<?php echo esc_attr( $sale_price_dates_to ); ?>" | ||
maxlength="10" | ||
pattern="[0-9]{4}-(0[1-9]|1[012])-(0[1-9]|1[0-9]|2[0-9]|3[01])" | ||
placeholder="<?php echo esc_attr( $date_on_sale_to->get_placeholder() ); ?>" | ||
<?php echo $date_on_sale_to->is_required() ? 'required' : ''; ?>> | ||
</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prevent setting sale schedule dates in the past
Currently, vendors can select past dates for the sale schedule (date_on_sale_from
and date_on_sale_to
), which allows them to set discount prices in the past. This could lead to inconsistencies in pricing and potential issues with customer expectations.
To resolve this, you should restrict the date inputs to prevent selecting past dates. You can achieve this by adding the min
attribute to the input fields, setting it to today's date. Here's how you can modify the input fields:
<input
type="text"
name="<?php echo esc_attr( $date_on_sale_from->get_name() ); ?>"
class="dokan-form-control dokan-start-date"
value="<?php echo esc_attr( $sale_price_dates_from ); ?>"
maxlength="10"
pattern="[0-9]{4}-(0[1-9]|1[012])-(0[1-9]|1[0-9]|2[0-9]|3[01])"
+ min="<?php echo esc_attr( date( 'Y-m-d' ) ); ?>"
placeholder="<?php echo esc_attr( $date_on_sale_from->get_placeholder() ); ?>"
<?php echo $date_on_sale_from->is_required() ? 'required' : ''; ?>>
<input
type="text"
name="<?php echo esc_attr( $date_on_sale_to->get_name() ); ?>"
class="dokan-form-control dokan-end-date"
value="<?php echo esc_attr( $sale_price_dates_to ); ?>"
maxlength="10"
pattern="[0-9]{4}-(0[1-9]|1[012])-(0[1-9]|1[0-9]|2[0-9]|3[01])"
+ min="<?php echo esc_attr( date( 'Y-m-d' ) ); ?>"
placeholder="<?php echo esc_attr( $date_on_sale_to->get_placeholder() ); ?>"
<?php echo $date_on_sale_to->is_required() ? 'required' : ''; ?>>
This ensures that vendors cannot select a date earlier than today.
Committable suggestion skipped: line range outside the PR's diff.
# Conflicts: # assets/src/js/product-editor.js # includes/Admin/Notices/Manager.php # includes/Product/Manager.php # templates/products/edit-product-single.php # templates/products/tmpl-add-product-popup.php
…age after resolving merge conflict
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (7)
tests/pw/pages/noticeAndPromotionPage.ts (1)
19-22
: LGTM! Consider enhancing test readability.The simplified implementation is more straightforward and deterministic. The use of
notToHaveCount
effectively verifies the presence of notice elements.Consider improving test readability by extracting these assertions into a custom helper method:
async assertNoticeElementsPresent() { await this.notToHaveCount(selector.admin.dokan.notice.noticeDiv, 0); await this.notToHaveCount(selector.admin.dokan.notice.slider, 0); await this.notToHaveCount(selector.admin.dokan.notice.sliderPrev, 0); await this.notToHaveCount(selector.admin.dokan.notice.sliderNext, 0); }This would make the test more maintainable and self-documenting.
assets/src/js/product-editor.js (2)
25-25
: Avoid unnecessary aliasing ofthis
The alias
var self = this;
is unnecessary since arrow functions inheritthis
from their enclosing scope. Remove the alias and usethis
directly.🧰 Tools
🪛 Biome (1.9.4)
[error] 25-25: This aliasing of this is unnecessary.
Arrow functions inherits
this
from their enclosing scope.
Safe fix: Use this instead of an alias.(lint/complexity/noUselessThisAlias)
93-93
: Remove unused parameterself
fromproductFormValidation
The parameter
self
in theproductFormValidation
method is not used. Consider removing it for clarity.src/admin/pages/Settings.vue (1)
590-594
: Consider using a more flexible action handling pattern.The current implementation tightly couples the action handler to a specific section ID. This could become harder to maintain as more actions are added.
Consider using a strategy pattern:
-handleAction( sectionId ) { - if ( 'dokan_form_manager' === sectionId ) { - this.resetAllFields( sectionId ); - } -}, +const ACTION_HANDLERS = { + 'dokan_form_manager': function(sectionId) { + this.resetAllFields(sectionId); + } +}; + +handleAction(sectionId) { + const handler = ACTION_HANDLERS[sectionId]; + if (handler) { + handler.call(this, sectionId); + } +},includes/Assets.php (1)
Line range hint
1000-1084
: Consider moving variation-related strings to a separate configuration file.The large block of variation-related localization strings (lines 1061-1084) should be moved to a separate configuration file for better maintainability, as indicated by the TODO comment.
Consider creating a new file
includes/i18n/variation-strings.php
to hold these strings and import them here. This will:
- Improve code organization
- Make it easier to maintain and update strings
- Follow the separation of concerns principle
templates/products/edit/sections/general.php (2)
16-19
: Consider enhancing error handling displayWhile the code checks for WP_Error, it silently returns without informing the user about what went wrong.
$section = ProductFormFactory::get_section( 'general' ); if ( is_wp_error( $section ) ) { + dokan_get_template_part( 'global/dokan-error', '', array( 'deleted' => true, 'message' => $section->get_error_message() ) ); return; }
427-442
: Enhance wp_editor security settingsThe current wp_editor implementation could be more secure by limiting allowed HTML tags and protocols.
wp_editor( $short_description->get_value( $product ), $short_description->get_name(), apply_filters( 'dokan_product_short_description', [ 'editor_height' => 50, 'quicktags' => true, 'media_buttons' => false, 'teeny' => false, 'editor_class' => 'post_excerpt', 'required' => $short_description->is_required(), + 'tinymce' => [ + 'valid_elements' => 'p,span,b,strong,em,i,ul,ol,li,a[href|target]', + 'valid_children' => '+body[p|ul|ol]', + 'extended_valid_elements' => 'span[*]', + ], ] ) );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (18)
assets/src/js/product-editor.js
(4 hunks)dokan-class.php
(2 hunks)includes/Admin/Settings.php
(0 hunks)includes/Assets.php
(8 hunks)includes/Product/Hooks.php
(3 hunks)includes/Product/Manager.php
(15 hunks)includes/ProductCategory/Helper.php
(4 hunks)includes/REST/Manager.php
(1 hunks)includes/functions.php
(2 hunks)src/admin/components/Fields.vue
(2 hunks)src/admin/pages/Settings.vue
(6 hunks)templates/products/edit/sections/general.php
(1 hunks)tests/pw/pages/noticeAndPromotionPage.ts
(1 hunks)tests/pw/pages/settingsPage.ts
(0 hunks)tests/pw/utils/dbData.ts
(0 hunks)tests/pw/utils/interfaces.ts
(7 hunks)tests/pw/utils/schemas.ts
(1 hunks)tests/pw/utils/testData.ts
(8 hunks)
💤 Files with no reviewable changes (3)
- tests/pw/pages/settingsPage.ts
- includes/Admin/Settings.php
- tests/pw/utils/dbData.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- dokan-class.php
- src/admin/components/Fields.vue
- tests/pw/utils/interfaces.ts
- tests/pw/utils/schemas.ts
🧰 Additional context used
🪛 GitHub Check: Run PHPCS inspection
includes/Assets.php
[warning] 582-582:
json_encode() is discouraged. Use wp_json_encode() instead.
includes/Product/Manager.php
[failure] 588-588:
Blank line found at start of control structure
🪛 Biome (1.9.4)
assets/src/js/product-editor.js
[error] 25-25: This aliasing of this is unnecessary.
Arrow functions inherits this
from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
tests/pw/utils/testData.ts
[error] 230-230: This property value named description is later overwritten by an object member with the same name.
Overwritten with this value.
If an object property with the same name is defined multiple times (except when combining a getter with a setter), only the last definition makes it into the object and previous definitions are ignored.
Unsafe fix: Remove this property value named description
(lint/suspicious/noDuplicateObjectKeys)
[error] 244-244: This property value named description is later overwritten by an object member with the same name.
Overwritten with this value.
If an object property with the same name is defined multiple times (except when combining a getter with a setter), only the last definition makes it into the object and previous definitions are ignored.
Unsafe fix: Remove this property value named description
(lint/suspicious/noDuplicateObjectKeys)
[error] 265-266: This property value named description is later overwritten by an object member with the same name.
Overwritten with this value.
If an object property with the same name is defined multiple times (except when combining a getter with a setter), only the last definition makes it into the object and previous definitions are ignored.
Unsafe fix: Remove this property value named description
(lint/suspicious/noDuplicateObjectKeys)
[error] 279-280: This property value named description is later overwritten by an object member with the same name.
Overwritten with this value.
If an object property with the same name is defined multiple times (except when combining a getter with a setter), only the last definition makes it into the object and previous definitions are ignored.
Unsafe fix: Remove this property value named description
(lint/suspicious/noDuplicateObjectKeys)
[error] 300-301: This property value named description is later overwritten by an object member with the same name.
Overwritten with this value.
If an object property with the same name is defined multiple times (except when combining a getter with a setter), only the last definition makes it into the object and previous definitions are ignored.
Unsafe fix: Remove this property value named description
(lint/suspicious/noDuplicateObjectKeys)
[error] 324-325: This property value named description is later overwritten by an object member with the same name.
Overwritten with this value.
If an object property with the same name is defined multiple times (except when combining a getter with a setter), only the last definition makes it into the object and previous definitions are ignored.
Unsafe fix: Remove this property value named description
(lint/suspicious/noDuplicateObjectKeys)
[error] 342-343: This property value named description is later overwritten by an object member with the same name.
Overwritten with this value.
If an object property with the same name is defined multiple times (except when combining a getter with a setter), only the last definition makes it into the object and previous definitions are ignored.
Unsafe fix: Remove this property value named description
(lint/suspicious/noDuplicateObjectKeys)
[error] 365-366: This property value named description is later overwritten by an object member with the same name.
Overwritten with this value.
If an object property with the same name is defined multiple times (except when combining a getter with a setter), only the last definition makes it into the object and previous definitions are ignored.
Unsafe fix: Remove this property value named description
(lint/suspicious/noDuplicateObjectKeys)
🔇 Additional comments (12)
includes/ProductCategory/Helper.php (1)
191-193
: Optimize array merging inside the loop to improve performance
Using array_merge
inside a loop can lead to performance issues with large datasets. Consider accumulating the ancestors in a temporary array and merging them after the loop.
includes/Product/Manager.php (2)
695-695
: Remove the unnecessary blank line at the start of the control structure
As per coding standards, remove the blank line at the start of the control structure to maintain code consistency.
443-450
:
Add error handling for product deletion
Ensure that the product deletion process includes error handling to manage potential issues when deleting the product.
includes/REST/Manager.php (1)
145-145
:
Verify impact of changing action hook parameters
The second parameter passed to do_action( 'dokan_new_product_added', $object->get_id(), [] );
has been changed from $request
to an empty array. This may affect hooked functions that rely on $request
. Ensure that all hooked functions are updated to handle this change to prevent potential issues.
Run the following script to identify functions hooked to dokan_new_product_added
that may be affected:
includes/Product/Hooks.php (1)
374-387
:
Verify consistency of the set_new_product_email_status
method signature
The method set_new_product_email_status
has an updated signature. Ensure that all calls to this method are updated accordingly, and the documentation reflects the new parameters and return type to prevent any errors.
Run the following script to find all invocations of set_new_product_email_status
:
✅ Verification successful
Let me try to find any references to this method in the codebase to verify its usage and previous signature. The previous command failed because the --php
flag is not supported by ripgrep. I've simplified the search and added context lines to better understand the method's implementation history.
I need to verify if there was actually a change in the method signature and understand how this hook is being used in the codebase to ensure consistency.
Method signature is consistent with its usage
The method set_new_product_email_status
is only used as a hook callback registered via add_filter('dokan_product_edit_meta_data', ...)
with 3 parameters, which matches exactly with the current method signature. The hook is properly applied in includes/Dashboard/Templates/Products.php
with all three parameters ($meta_data
, $product
, $is_new_product
). No inconsistencies were found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all calls to 'set_new_product_email_status' and check their parameters.
# Search for calls to 'set_new_product_email_status' in the codebase
rg "set_new_product_email_status\s*\(" --php
Length of output: 454
Script:
#!/bin/bash
# Search for calls to 'set_new_product_email_status' in the codebase
rg "set_new_product_email_status"
# Also search for the method definition to understand its previous signature
rg -B 5 -A 5 "function set_new_product_email_status"
Length of output: 1012
Script:
#!/bin/bash
# Let's check the git history of this file to see if the method signature was changed
git log -L :set_new_product_email_status:includes/Product/Hooks.php
# Also check how this hook is used in the codebase
rg "dokan_product_edit_meta_data" -B 2 -A 2
Length of output: 1531
src/admin/pages/Settings.vue (2)
58-60
: LGTM: Action button implementation is clean and follows Vue best practices.
The conditional rendering and event handling are implemented correctly.
596-613
: 🛠️ Refactor suggestion
Improve resetAllFields implementation for better performance and reliability.
The current implementation has several potential issues:
- Uses JSON parse/stringify for cloning which doesn't handle special values
- Nested loops could impact performance with large datasets
- No error handling for malformed data
Consider this improved implementation:
resetAllFields(sectionId) {
+ try {
- let settingFields = Object.entries(this.settingFields[sectionId]);
- let clonedSectionSettingValues = JSON.parse(JSON.stringify(this.settingValues[sectionId]));
+ const clonedSectionSettingValues = structuredClone(this.settingValues[sectionId]);
+ const settingFields = this.settingFields[sectionId];
- settingFields.forEach((key, value) => {
- let blockId = key[0];
+ Object.entries(settingFields).forEach(([blockId, sectionValue]) => {
+ if (!blockId || !sectionValue?.fields) return;
- if (blockId) {
- let sectionValue = key[1];
- for (const [FieldId, fieldData] of Object.entries(sectionValue.fields)) {
- clonedSectionSettingValues[blockId]['fields'][FieldId]['visibility'] = fieldData.default.visibility;
- clonedSectionSettingValues[blockId]['fields'][FieldId]['required'] = fieldData.default.required;
- }
+ Object.entries(sectionValue.fields).forEach(([fieldId, fieldData]) => {
+ if (!fieldData?.default) return;
+
+ clonedSectionSettingValues[blockId].fields[fieldId] = {
+ ...clonedSectionSettingValues[blockId].fields[fieldId],
+ visibility: fieldData.default.visibility,
+ required: fieldData.default.required
+ };
+ });
+ });
- });
this.settingValues[sectionId] = clonedSectionSettingValues;
+ } catch (error) {
+ console.error('Failed to reset fields:', error);
+ // Consider showing user-friendly error message
}
},
Likely invalid or redundant comment.
includes/functions.php (2)
566-566
: Improved required attribute validation
The change from isset()
to !empty()
is a good improvement as it handles both existence and non-empty value checks.
616-630
: Enhanced checkbox input with description tooltip
Good addition of tooltip functionality for checkbox descriptions. This improves user experience by providing contextual help.
The implementation uses Font Awesome icons and proper ARIA attributes for accessibility.
templates/products/edit/sections/general.php (3)
485-489
: LGTM! Proper security measures in place.
The form submission implementation includes proper nonce verification and escaping.
173-182
: 🛠️ Refactor suggestion
Add client-side validation for sale dates
The sale schedule implementation should prevent setting end dates before start dates and past dates.
+<script>
+document.addEventListener('DOMContentLoaded', function() {
+ const startDate = document.querySelector('.dokan-start-date');
+ const endDate = document.querySelector('.dokan-end-date');
+
+ if (startDate && endDate) {
+ startDate.addEventListener('change', function() {
+ endDate.min = this.value;
+ });
+
+ endDate.addEventListener('change', function() {
+ if (this.value < startDate.value) {
+ this.value = startDate.value;
+ }
+ });
+
+ // Set minimum date to today
+ const today = new Date().toISOString().split('T')[0];
+ startDate.min = today;
+ endDate.min = today;
+ }
+});
+</script>
Likely invalid or redundant comment.
377-401
: 🛠️ Refactor suggestion
Enhance gallery images validation and sanitization
The current implementation needs additional checks for image IDs and proper array handling.
-$product_images = is_array( $product_images ) ? $product_images : explode( ',', $product_images );
+$product_images = is_array( $product_images ) ? array_filter( array_map( 'absint', $product_images ) ) : array_filter( array_map( 'absint', explode( ',', $product_images ) ) );
if ( $product_images ) :
foreach ( $product_images as $image_id ) :
- if ( empty( $image_id ) ) :
+ if ( empty( $image_id ) || ! wp_attachment_is_image( $image_id ) ) :
continue;
endif;
Likely invalid or redundant comment.
public function get_product_fields() { | ||
$temp_fields = []; | ||
foreach ( ProductFormFactory::get_fields() as $field_id => $field ) { | ||
$temp_fields[ $field_id ] = $field->toArray(); | ||
} | ||
|
||
return json_encode( $temp_fields ); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use wp_json_encode for WordPress compatibility.
The current implementation uses PHP's native json_encode()
. WordPress provides wp_json_encode()
which handles additional edge cases and ensures better compatibility.
Apply this fix:
public function get_product_fields() {
$temp_fields = [];
foreach ( ProductFormFactory::get_fields() as $field_id => $field ) {
$temp_fields[ $field_id ] = $field->toArray();
}
- return json_encode( $temp_fields );
+ return wp_json_encode( $temp_fields );
}
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
public function get_product_fields() { | |
$temp_fields = []; | |
foreach ( ProductFormFactory::get_fields() as $field_id => $field ) { | |
$temp_fields[ $field_id ] = $field->toArray(); | |
} | |
return json_encode( $temp_fields ); | |
} | |
public function get_product_fields() { | |
$temp_fields = []; | |
foreach ( ProductFormFactory::get_fields() as $field_id => $field ) { | |
$temp_fields[ $field_id ] = $field->toArray(); | |
} | |
return wp_json_encode( $temp_fields ); | |
} |
🧰 Tools
🪛 GitHub Check: Run PHPCS inspection
[warning] 582-582:
json_encode() is discouraged. Use wp_json_encode() instead.
@@ -227,6 +227,7 @@ export const data = { | |||
productName: () => `${faker.commerce.productName()}_${faker.string.nanoid(5)} (Simple)`, | |||
category: 'Uncategorized', | |||
regularPrice: () => faker.finance.amount({ min: 100, max: 200, dec: faker.helpers.arrayElement([1, 2]) }).replace('.', ','), | |||
description: 'test long description', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix duplicate description
properties in product type objects
Each product type object has duplicate description
properties which can lead to unexpected behavior since the second property will overwrite the first one.
Apply this fix to consolidate the descriptions:
simple: {
productType: 'simple',
productName: () => `${faker.commerce.productName()}_${faker.string.nanoid(5)} (Simple)`,
category: 'Uncategorized',
regularPrice: () => faker.finance.amount({ min: 100, max: 200, dec: faker.helpers.arrayElement([1, 2]) }).replace('.', ','),
- description: 'test long description',
storeName: `${VENDOR}store`,
status: 'publish',
stockStatus: false,
editProduct: '',
description: 'test description',
saveSuccessMessage: 'Success! The product has been saved successfully. View Product →',
}
Apply similar fixes to other product type objects (downloadable, virtual, variable, external, simpleSubscription, variableSubscription, vendorSubscription).
Also applies to: 244-244, 265-265, 279-279, 300-300, 324-324, 342-342, 365-365
🧰 Tools
🪛 Biome (1.9.4)
[error] 230-230: This property value named description is later overwritten by an object member with the same name.
Overwritten with this value.
If an object property with the same name is defined multiple times (except when combining a getter with a setter), only the last definition makes it into the object and previous definitions are ignored.
Unsafe fix: Remove this property value named description
(lint/suspicious/noDuplicateObjectKeys)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/pw/utils/testData.ts (1)
242-244
:⚠️ Potential issueFix duplicate
description
properties in product type objectsEach product type object has duplicate
description
properties which can lead to unexpected behavior since the second property will overwrite the first one.Apply this fix to consolidate the descriptions:
simple: { productType: 'simple', productName: () => `${faker.commerce.productName()}_${faker.string.nanoid(5)} (Simple)`, category: 'Uncategorized', regularPrice: () => faker.finance.amount({ min: 100, max: 200, dec: faker.helpers.arrayElement([1, 2]) }).replace('.', ','), - description: 'test long description', storeName: `${VENDOR}store`, status: 'publish', stockStatus: false, editProduct: '', description: 'test description', saveSuccessMessage: 'Success! The product has been saved successfully. View Product →', }
Apply similar fixes to other product type objects (downloadable, virtual, variable, external, simpleSubscription, variableSubscription, vendorSubscription).
Also applies to: 256-258, 277-279, 291-293, 313-315, 337-339, 355-357, 379-381
🧰 Tools
🪛 Biome (1.9.4)
[error] 242-242: This property value named description is later overwritten by an object member with the same name.
Overwritten with this value.
If an object property with the same name is defined multiple times (except when combining a getter with a setter), only the last definition makes it into the object and previous definitions are ignored.
Unsafe fix: Remove this property value named description(lint/suspicious/noDuplicateObjectKeys)
🧹 Nitpick comments (1)
tests/pw/utils/testData.ts (1)
Line range hint
1-1000
: Consider improving the test data structure for better maintainabilityThe test data file could benefit from the following improvements:
- Extract common strings into constants to avoid repetition
- Add TypeScript interfaces for better type safety
- Split the large data object into smaller, more focused modules
Example implementation:
// constants.ts export const DESCRIPTIONS = { TEST: 'test description', TEST_LONG: 'test long description', // ... other common strings }; // interfaces.ts interface ProductBase { productType: string; productName: () => string; category: string; description: string; // ... other common properties } interface SimpleProduct extends ProductBase { stockStatus: boolean; // ... simple product specific properties } // productData.ts export const simpleProduct: SimpleProduct = { productType: 'simple', productName: () => `${faker.commerce.productName()}_${faker.string.nanoid(5)} (Simple)`, category: 'Uncategorized', description: DESCRIPTIONS.TEST, // ... other properties };This structure would:
- Reduce code duplication
- Improve type safety
- Make the code more maintainable
- Make it easier to update common values
🧰 Tools
🪛 Biome (1.9.4)
[error] 242-242: This property value named description is later overwritten by an object member with the same name.
Overwritten with this value.
If an object property with the same name is defined multiple times (except when combining a getter with a setter), only the last definition makes it into the object and previous definitions are ignored.
Unsafe fix: Remove this property value named description(lint/suspicious/noDuplicateObjectKeys)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
dokan-class.php
(2 hunks)includes/Product/Hooks.php
(3 hunks)tests/pw/utils/dbData.ts
(0 hunks)tests/pw/utils/interfaces.ts
(7 hunks)tests/pw/utils/schemas.ts
(1 hunks)tests/pw/utils/testData.ts
(8 hunks)
💤 Files with no reviewable changes (1)
- tests/pw/utils/dbData.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- dokan-class.php
- tests/pw/utils/schemas.ts
- tests/pw/utils/interfaces.ts
🧰 Additional context used
🪛 Biome (1.9.4)
tests/pw/utils/testData.ts
[error] 242-242: This property value named description is later overwritten by an object member with the same name.
Overwritten with this value.
If an object property with the same name is defined multiple times (except when combining a getter with a setter), only the last definition makes it into the object and previous definitions are ignored.
Unsafe fix: Remove this property value named description
(lint/suspicious/noDuplicateObjectKeys)
[error] 256-256: This property value named description is later overwritten by an object member with the same name.
Overwritten with this value.
If an object property with the same name is defined multiple times (except when combining a getter with a setter), only the last definition makes it into the object and previous definitions are ignored.
Unsafe fix: Remove this property value named description
(lint/suspicious/noDuplicateObjectKeys)
[error] 277-278: This property value named description is later overwritten by an object member with the same name.
Overwritten with this value.
If an object property with the same name is defined multiple times (except when combining a getter with a setter), only the last definition makes it into the object and previous definitions are ignored.
Unsafe fix: Remove this property value named description
(lint/suspicious/noDuplicateObjectKeys)
[error] 291-292: This property value named description is later overwritten by an object member with the same name.
Overwritten with this value.
If an object property with the same name is defined multiple times (except when combining a getter with a setter), only the last definition makes it into the object and previous definitions are ignored.
Unsafe fix: Remove this property value named description
(lint/suspicious/noDuplicateObjectKeys)
[error] 313-314: This property value named description is later overwritten by an object member with the same name.
Overwritten with this value.
If an object property with the same name is defined multiple times (except when combining a getter with a setter), only the last definition makes it into the object and previous definitions are ignored.
Unsafe fix: Remove this property value named description
(lint/suspicious/noDuplicateObjectKeys)
[error] 337-338: This property value named description is later overwritten by an object member with the same name.
Overwritten with this value.
If an object property with the same name is defined multiple times (except when combining a getter with a setter), only the last definition makes it into the object and previous definitions are ignored.
Unsafe fix: Remove this property value named description
(lint/suspicious/noDuplicateObjectKeys)
[error] 355-356: This property value named description is later overwritten by an object member with the same name.
Overwritten with this value.
If an object property with the same name is defined multiple times (except when combining a getter with a setter), only the last definition makes it into the object and previous definitions are ignored.
Unsafe fix: Remove this property value named description
(lint/suspicious/noDuplicateObjectKeys)
[error] 379-380: This property value named description is later overwritten by an object member with the same name.
Overwritten with this value.
If an object property with the same name is defined multiple times (except when combining a getter with a setter), only the last definition makes it into the object and previous definitions are ignored.
Unsafe fix: Remove this property value named description
(lint/suspicious/noDuplicateObjectKeys)
🔇 Additional comments (2)
includes/Product/Hooks.php (2)
9-9
: LGTM! Security enhancement added.
The addition of the direct script access prevention check follows WordPress security best practices.
375-388
: Consider the previously suggested optimization for better readability.
The implementation is functionally correct, but could be more concise as suggested in the previous review.
Apply this optimization:
- public function set_new_product_email_status( array $meta_data, WC_Product $product, bool $is_new_product ) {
- if ( ! $is_new_product ) {
- return $meta_data;
- }
-
- $meta_data['_dokan_new_product_email_sent'] = 'no';
-
- return $meta_data;
- }
+ public function set_new_product_email_status( array $meta_data, WC_Product $product, bool $is_new_product ) {
+ if ( $is_new_product ) {
+ $meta_data['_dokan_new_product_email_sent'] = 'no';
+ }
+ return $meta_data;
+ }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (2)
includes/wc-functions.php (2)
34-37
: Consider optimizing the data cleaning sequence.The current implementation cleans the data twice:
wp_unslash()
on line 35wc_clean()
on line 37This could be optimized for better performance.
- $data = apply_filters( 'dokan_restrict_product_gallery_images_on_edit', wp_unslash( $data['gallery_image_ids'] ) ); - $attachment_ids = array_filter( explode( ',', wc_clean( $data ) ) ); + $data = wc_clean( wp_unslash( $data['gallery_image_ids'] ) ); + $data = apply_filters( 'dokan_restrict_product_gallery_images_on_edit', $data ); + $attachment_ids = array_filter( explode( ',', $data ) );
Line range hint
305-315
: Enhance error logging information.The current error logging is too generic and might not provide enough context for debugging.
try { wc_update_product_stock_status( $post_id, $stock_status ); } catch ( Exception $ex ) { - dokan_log( 'product stock update exception' ); + dokan_log( sprintf( 'Failed to update stock status for product #%d: %s', $post_id, $ex->getMessage() ) ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
includes/Dashboard/Templates/Products.php
(9 hunks)includes/wc-functions.php
(4 hunks)
🧰 Additional context used
🪛 GitHub Check: Run PHPCS inspection
includes/Dashboard/Templates/Products.php
[failure] 494-494:
Detected usage of a non-sanitized input variable: $_POST['gallery_image_ids']
🔇 Additional comments (7)
includes/wc-functions.php (2)
Line range hint 305-315
: LGTM! Improved stock management with error handling.
The changes add proper error handling and prevent unnecessary stock updates.
Line range hint 317-329
: LGTM! Efficient stock updates with proper handling of variable products.
The changes optimize database operations by:
- Only updating stock when it has changed
- Properly handling variable products with different update logic
includes/Dashboard/Templates/Products.php (5)
28-45
: Convert static properties to instance properties
Using static properties for instance-specific data can lead to race conditions in concurrent scenarios.
57-70
: LGTM! Well-organized hook setup
The constructor properly organizes action hooks by functionality, making the code maintainable and easy to understand.
136-145
: Replace informal error message
The error message "Are you cheating?" should be replaced with a more professional message.
Line range hint 194-312
: LGTM! Robust template loading implementation
The template loading methods consistently check for product existence and section visibility before rendering.
530-569
: LGTM! Well-structured product update handling
The product update logic includes:
- Proper hooks for extensibility
- Comprehensive field validation
- Appropriate error handling
$attachment_ids = isset( $_POST['gallery_image_ids'] ) ? apply_filters( 'dokan_restrict_product_gallery_images_on_edit', wp_unslash( $_POST['gallery_image_ids'] ) ) : ''; | ||
$attachment_ids = array_filter( explode( ',', wc_clean( $attachment_ids ) ) ); | ||
$field_value = $field->sanitize( implode( ',', $attachment_ids ), $product ); //phpcs:ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sanitize gallery image IDs before processing
The gallery image IDs from POST data should be sanitized before processing to prevent potential security issues.
Apply this diff to properly sanitize the input:
-$attachment_ids = isset( $_POST['gallery_image_ids'] ) ? apply_filters( 'dokan_restrict_product_gallery_images_on_edit', wp_unslash( $_POST['gallery_image_ids'] ) ) : '';
+$attachment_ids = isset( $_POST['gallery_image_ids'] ) ? apply_filters( 'dokan_restrict_product_gallery_images_on_edit', sanitize_text_field( wp_unslash( $_POST['gallery_image_ids'] ) ) ) : '';
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
$attachment_ids = isset( $_POST['gallery_image_ids'] ) ? apply_filters( 'dokan_restrict_product_gallery_images_on_edit', wp_unslash( $_POST['gallery_image_ids'] ) ) : ''; | |
$attachment_ids = array_filter( explode( ',', wc_clean( $attachment_ids ) ) ); | |
$field_value = $field->sanitize( implode( ',', $attachment_ids ), $product ); //phpcs:ignore | |
$attachment_ids = isset( $_POST['gallery_image_ids'] ) ? apply_filters( 'dokan_restrict_product_gallery_images_on_edit', sanitize_text_field( wp_unslash( $_POST['gallery_image_ids'] ) ) ) : ''; | |
$attachment_ids = array_filter( explode( ',', wc_clean( $attachment_ids ) ) ); | |
$field_value = $field->sanitize( implode( ',', $attachment_ids ), $product ); //phpcs:ignore |
🧰 Tools
🪛 GitHub Check: Run PHPCS inspection
[failure] 494-494:
Detected usage of a non-sanitized input variable: $_POST['gallery_image_ids']
All Submissions:
Changes proposed in this Pull Request:
Related Pull Request(s)
https://github.com/getdokan/dokan-pro/pull/2883
Closes
How to test the changes in this Pull Request:
Changelog entry
Title
Detailed Description of the pull request. What was previous behaviour
and what will be changed in this PR.
Before Changes
Describe the issue before changes with screenshots(s).
After Changes
Describe the issue after changes with screenshot(s).
Feature Video (optional)
Link of detailed video if this PR is for a feature.
PR Self Review Checklist:
FOR PR REVIEWER ONLY:
Summary by CodeRabbit
Release Notes
New Features
description
property for various product types, enhancing product detail visibility.Bug Fixes
required
attribute in input fields for better validation.Style
Documentation
Refactor
Chores